Refactor reward account handling to use StakeAddress #250

lowhung · 2025-10-14T23:46:15Z

Description of Changes

Resolves issue #163

Tx unpacker takes care of the network id and constructing the StakeAddress structs, and is the only module that needs to be aware of the network id.
Still some places where I'd like to unify certain types like NetworkId vs AddressNetwork, StakeAddressPayload vs StakeCredential etc. but those can be done in a follow-up PR.
Replaced RewardAccount with StakeAddress across modules for consistency.
Updated serialization, deserialization, and encoding/decoding logic to align with StakeAddress.
Adjusted tests, helpers, and corresponding comments to support the refactor.

- Replace `RewardAccount` with `StakeAddress` across modules for consistency. - Adapt serialization, deserialization, and encoding/decoding logic for `StakeAddress`. - Adjust related tests, helpers, and comments to align with the refactor.

- Simplify `reward_account` handling by utilizing `StakeAddress::from_binary`. - Adjust `StakeAddress` default implementation for consistency.

…tion - Replace `reward_account` field with `StakeAddress::default()` for consistency. - Remove redundant `StakeAddressPayload` in test cases. - Clean up unused imports and redundant code across modules.

…t` initialization

common/src/address.rs

- Replace `RewardAccount` with `StakeAddress` across modules for consistency. - Adapt serialization, deserialization, and encoding/decoding logic for `StakeAddress`. - Adjust related tests, helpers, and comments to align with the refactor. # Conflicts: # modules/accounts_state/src/rewards.rs # modules/accounts_state/src/snapshot.rs # modules/accounts_state/src/state.rs

- Simplify `reward_account` handling by utilizing `StakeAddress::from_binary`. - Adjust `StakeAddress` default implementation for consistency.

…tion - Replace `reward_account` field with `StakeAddress::default()` for consistency. - Remove redundant `StakeAddressPayload` in test cases. - Clean up unused imports and redundant code across modules.

…t` initialization

- Replace `StakeAddress::from_binary` usage with `reward_account.get_hash` for simplicity and consistency. - Update all modules to align with the updated `StakeAddress` handling. - Remove redundant error handling and simplify logging where applicable.

…nt-with-stake-address' into lowhung/163-replace-reward-account-with-stake-address # Conflicts: # modules/accounts_state/src/rewards.rs # modules/accounts_state/src/snapshot.rs # modules/accounts_state/src/state.rs

…ify related logic - Update `RewardDetail` and reward handling logic to use `StakeAddress` directly. - Replace `reward_account` field with `.get_hash()` where necessary.

…eded. - Update imports and logic across modules to use `StakeAddress` exclusively.

Copilot

Pull Request Overview

This PR refactors the codebase to replace the RewardAccount type (which was a Vec<u8>) with the more structured StakeAddress type for improved type safety and consistency across the application.

Key Changes:

Replaced RewardAccount type alias with StakeAddress throughout the codebase
Updated serialization/deserialization logic to use StakeAddress methods
Modified test fixtures to use StakeAddress::default() instead of empty vectors

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
modules/tx_unpacker/src/map_parameters.rs	Updated certificate and proposal mapping to deserialize reward accounts using `StakeAddress::from_binary()`
modules/spo_state/src/state.rs	Updated test fixtures to use `StakeAddress::default()` instead of `vec![0]` and added import
modules/rest_blockfrost/src/handlers/pools.rs	Changed reward account conversion from `to_bech32_with_hrp()` to `to_string()` method
modules/accounts_state/src/verifier.rs	Updated verifier to use `StakeAddress::get_hash()` for comparisons and logging
modules/accounts_state/src/state.rs	Refactored SPO retirement logic to use `StakeAddress` directly, updated reward processing to use `get_hash()`, and fixed comment grammar
modules/accounts_state/src/snapshot.rs	Simplified reward account registration check by removing error handling and using `StakeAddress` directly
modules/accounts_state/src/rewards.rs	Updated reward calculation to work with `StakeAddress`, replaced hash comparisons with `get_hash()` calls
common/src/types.rs	Removed `RewardAccount` type alias and updated `PoolRegistration` and `ProposalProcedure` to use `StakeAddress`
common/src/address.rs	Added CBOR encoding/decoding implementations, `Default` trait, reorganized methods, and added comprehensive tests for `StakeAddress`

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

common/src/address.rs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

modules/accounts_state/src/rewards.rs

…y creation. - Update reward handling to use `from_stake_key_hash` with `NetworkId`. - Implement `From<NetworkId>` for `AddressNetwork` for easier conversion.

…nt-with-stake-address' into lowhung/163-replace-reward-account-with-stake-address # Conflicts: # common/src/address.rs

…dling across modules. - Update reward processing, delegation, and state management to use `StakeAddress`. - Simplify APIs and adjust implementations to process `StakeAddress` directly. - Refactor tests to reflect new `StakeAddress`-based structure. - Enhance error logging to include full stake address information.

…pansion, and minor documentation fixes - Rename `stake_addresses` function parameters for clarity and consistency (`stake_addresses` -> `stake_keys`). - Adjust test cases to include comprehensive workflows for registration, delegation, withdrawal, and state updates. - Add new utility test modules to validate delegation and reward processes. - Minor improvements in comment documentation for public functions.

sandtreader

Really good, you really opened a rabbit hole here and have chased it (almost) to the end :-)

The only major problem I can see is that converting a StakeCredential to a StakeAddress (passing None) assumes mainnet, which isn't the case. I think we need to chase further and potentially replace StakeCredential with StakeAddress, and deal with where they are created (who hopefully will know which network they are on).

Also thanks for adding all the tests to stake_addresses.rs!

sandtreader · 2025-10-20T09:52:46Z

common/src/address.rs

-            StakeAddressPayload::StakeKeyHash(data) => (data, 0b1110),
-            StakeAddressPayload::ScriptHash(data) => (data, 0b1111),
-        };
+impl PartialEq for StakeAddress {


Not sure why you want to limit the comparison to just hashes - the derived one would have included the network and type.

That's a good point, I'll remove this and use the derived version instead -- especially since my Borrow implementation I refer to here doesn't work.

I removed it here, we'll use the derived one now. https://github.com/input-output-hk/acropolis/pull/250/files#diff-5972d21f70eced49c6daf5ebec0b9949b8338a38a9ed89c65eb1004ffaffb2a2R211

common/src/address.rs

sandtreader · 2025-10-20T09:57:59Z

common/src/address.rs

+    }
+}
+
+impl Default for StakeAddress {


I'm always a bit nervous providing Default for things that don't have a meaningful one, but I guess this is forced on us by use downstream (PoolRegistration?). We should maybe investigate why that needs a Default...

Another good point here -- as you said it's enforced on us by the PoolRegistgration

impl HistoricalSPOState { #[allow(dead_code)] pub fn new(store_config: &StoreConfig) -> Self { Self { registration: store_config.store_registration.then(PoolRegistration::default), // <-- default here updates: store_config.store_updates.then(Vec::new), delegators: store_config.store_delegators.then(HashSet::new), votes: store_config.store_votes.then(Vec::new), blocks: store_config.store_blocks.then(OrdMap::new), } } // ... } ```

This seems to be enforced due to the SPO / Ledger state work that decodes the cbor.

sandtreader · 2025-10-20T12:23:12Z

common/src/stake_addresses.rs

 #[derive(Default, Debug)]
 pub struct StakeAddressMap {
-    inner: HashMap<KeyHash, StakeAddressState>,
+    inner: HashMap<StakeAddress, StakeAddressState>,


This is interesting, because StakeAddress includes the network ID. I'm not sure it makes any sense to allow a mixture of networks in a single instance of (say) AccountsState, so this could in theory be reduced to a StakeAddressPayload, saving a couple of bytes. However that makes it impossible regenerate a StakeAddress for reward payouts which I assume is the reason for switching this, so it's probably the right thing - just a note.

However that makes it impossible regenerate a StakeAddress for reward payouts which I assume is the reason for switching this, so it's probably the right thing - just a note.

This is exactly why I needed to opt for the full StakeAddress struct as the key here.

sandtreader · 2025-10-20T12:30:09Z

common/src/stake_addresses.rs

+        )
+    }
+
+    mod registration_tests {


Yes, like this splitting of tests!

sandtreader · 2025-10-20T12:53:02Z

modules/accounts_state/src/state.rs

-                deregistrations.iter().for_each(|k| debug!("Deregistration {}", hex::encode(k)));
+                registrations
+                    .iter()
+                    .for_each(|addr| debug!("Registration {}", hex::encode(addr.get_hash())));


More use-cases for Display in StakeAddress

sandtreader · 2025-10-20T12:54:16Z

modules/accounts_state/src/state.rs

                    let mut total_value: u64 = 0;
                    for (credential, value) in deltas.iter() {
-                        let hash = credential.get_hash();
+                        let stake_address = credential.to_stake_address(None); // Need to convert credential to address


OK, this effectively assumes mainnet, which we can't assume... It feels like where we use StakeCredential we should be using StakeAddress too, and it'll be up to whoever creates them to make sure the network is set right.

sandtreader · 2025-10-20T12:55:50Z

modules/accounts_state/src/state.rs

-    /// Register a stake address, with specified deposit if known
+    /// Register a stake address, with a specified deposit if known
    fn register_stake_address(&mut self, credential: &StakeCredential, deposit: Option<Lovelace>) {
+        let stake_address = credential.to_stake_address(None); // Convert credential to full address


Assumes mainnet - see above

sandtreader · 2025-10-20T12:56:03Z

modules/accounts_state/src/state.rs


    /// Deregister a stake address, with specified refund if known
    fn deregister_stake_address(&mut self, credential: &StakeCredential, refund: Option<Lovelace>) {
+        let stake_address = credential.to_stake_address(None); // Convert credential to full address


Assumes mainnet - see above

sandtreader · 2025-10-20T13:09:05Z

modules/spo_state/src/state.rs

        };
        let mut stake_addresses = stake_addresses.lock().unwrap();
-        stake_addresses.register_stake_address(credential);
+        stake_addresses.register_stake_address(&credential.to_stake_address(None));


Assumption of Mainnet again

to_stake_address no longer exists. All of the StakeAddress construction can happen in the tx unpacker now, and that's the only place that configures the network-id.

lowhung · 2025-10-20T20:32:36Z

Todos:

Implement Display pattern on StakeAddress
Store network in BlockInfo and pass to stake address creation
Remove Borrow, ensure &[StakeAddress] always passed into stake address map

… display and improve `StakeAddress` implementation - Update logging to use `StakeAddress` directly instead of `hex::encode(...get_hash())` for better readability. - Implement `Display` trait for `StakeAddress` to simplify its usage in logs and messages. - Adjust `PartialEq` implementation: include `network` and `payload` for accuracy. - Refactor related tests and code to align with the updated `StakeAddress`.

…ied handling. - Updated APIs, queries, and function parameters to use `StakeAddress`. - Refactored tests and adjusted utility functions for compatibility. - Improved code readability and consistency by streamlining `StakeAddress` usage.

…ding - Remove `From<NetworkId> for AddressNetwork` implementation for cleanup. - Simplify logging by directly using `StakeAddress` instead of `hex::encode(...get_hash())`. - Add `TODO` comments for handling network in relevant functions. - Minor readability improvements in account state management and registration functions.

Resolves issue #163 This adds `NetworkId` into the `BlockInfo` struct, and introduces this identifier into the account state, drep state, epochs state and rest_blockfrost as these all rely on querying the `StakeAddressMap`.

…ddress` handling in tx_unpacker.rs - Removed `NetworkId` from structures and functions across modules. - Replaced `KeyHash` and `Credential` usage with `StakeAddress` for consistency.

…rgets and simplify handling

…r clarity.

…rd-account-with-stake-address-block-info-network-id Add `NetworkId` into `BlockInfo`

…vs hex encoding

…e-address # Conflicts: # common/src/address.rs # common/src/stake_addresses.rs # common/src/types.rs # modules/accounts_state/src/state.rs # modules/tx_unpacker/src/tx_unpacker.rs

lowhung added 4 commits October 14, 2025 16:45

Refactor StakeAddress usage and clean up redundant annotations

db99390

- Simplify `reward_account` handling by utilizing `StakeAddress::from_binary`. - Adjust `StakeAddress` default implementation for consistency.

Refactor StakeAddress usage to simplify reward_account initializa…

97f38c2

…tion - Replace `reward_account` field with `StakeAddress::default()` for consistency. - Remove redundant `StakeAddressPayload` in test cases. - Clean up unused imports and redundant code across modules.

Simplify StakeAddress::from_binary error handling in `reward_accoun…

f76615f

…t` initialization

lowhung commented Oct 15, 2025

View reviewed changes

common/src/address.rs Show resolved Hide resolved

lowhung added 6 commits October 14, 2025 17:49

Refactor StakeAddress usage and clean up redundant annotations

d033ba7

- Simplify `reward_account` handling by utilizing `StakeAddress::from_binary`. - Adjust `StakeAddress` default implementation for consistency.

Refactor StakeAddress usage to simplify reward_account initializa…

add6d5d

…tion - Replace `reward_account` field with `StakeAddress::default()` for consistency. - Remove redundant `StakeAddressPayload` in test cases. - Clean up unused imports and redundant code across modules.

Simplify StakeAddress::from_binary error handling in `reward_accoun…

3e45097

…t` initialization

lowhung force-pushed the lowhung/163-replace-reward-account-with-stake-address branch from 5b75210 to 7e1fe2c Compare October 15, 2025 02:41

Replace RewardAccount with StakeAddress for consistency and simpl…

c90a1c1

…ify related logic - Update `RewardDetail` and reward handling logic to use `StakeAddress` directly. - Replace `reward_account` field with `.get_hash()` where necessary.

lowhung force-pushed the lowhung/163-replace-reward-account-with-stake-address branch from 7e1fe2c to c90a1c1 Compare October 15, 2025 03:08

- Eliminate the RewardAccount type definition as it is no longer ne…

f625171

…eded. - Update imports and logic across modules to use `StakeAddress` exclusively.

lowhung force-pushed the lowhung/163-replace-reward-account-with-stake-address branch from 507f8ec to f625171 Compare October 15, 2025 16:51

lowhung requested review from buddhisthead, Copilot, golddydev, sandtreader and whankinsiv October 15, 2025 20:31

Copilot AI reviewed Oct 15, 2025

View reviewed changes

common/src/address.rs Show resolved Hide resolved

common/src/address.rs Outdated Show resolved Hide resolved

common/src/address.rs Outdated Show resolved Hide resolved

Add encode/decod, add comprehensive test cases for encoding, decoding

d4e7163

lowhung force-pushed the lowhung/163-replace-reward-account-with-stake-address branch from 7cf9747 to d4e7163 Compare October 15, 2025 20:33

Add encode/decod, add comprehensive test cases for encoding, decoding

37a5334

lowhung requested a review from Copilot October 15, 2025 21:25

This comment was marked as spam.

Sign in to view

Update common/src/address.rs

6ee41c8

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

lowhung requested a review from Copilot October 15, 2025 23:36

This comment was marked as spam.

Sign in to view

Add encode/decod, add comprehensive test cases for encoding, decoding

1ae4fea

lowhung commented Oct 16, 2025

View reviewed changes

modules/accounts_state/src/rewards.rs Show resolved Hide resolved

lowhung added 3 commits October 17, 2025 10:16

- Add from_stake_key_hash constructor for StakeAddress to simplif…

6d757e5

…y creation. - Update reward handling to use `from_stake_key_hash` with `NetworkId`. - Implement `From<NetworkId>` for `AddressNetwork` for easier conversion.

Merge remote-tracking branch 'origin/lowhung/163-replace-reward-accou…

cb6b823

…nt-with-stake-address' into lowhung/163-replace-reward-account-with-stake-address # Conflicts: # common/src/address.rs

lowhung force-pushed the lowhung/163-replace-reward-account-with-stake-address branch from ff594da to 497214e Compare October 17, 2025 23:16

sandtreader requested changes Oct 20, 2025

View reviewed changes

lowhung added 10 commits October 20, 2025 13:57

### Description

fe8c1c9

Resolves issue #163 This adds `NetworkId` into the `BlockInfo` struct, and introduces this identifier into the account state, drep state, epochs state and rest_blockfrost as these all rely on querying the `StakeAddressMap`.

Refactor: Remove NetworkId from BlockInfo and standardize `StakeA…

f8a97b4

…ddress` handling in tx_unpacker.rs - Removed `NetworkId` from structures and functions across modules. - Replaced `KeyHash` and `Credential` usage with `StakeAddress` for consistency.

Refactor: Replace StakeCredentials with StakeAddresses for MIR ta…

de08f0a

…rgets and simplify handling

Refactor: Standardize variable names (e.g., cred -> delegator) fo…

39e0f13

…r clarity.

Merge pull request #265 from input-output-hk/lowhung/163-replace-rewa…

2faea48

…rd-account-with-stake-address-block-info-network-id Add `NetworkId` into `BlockInfo`

Unstage files with simple import changes, use Display implementation …

85f3354

…vs hex encoding

Merge branch 'main' into lowhung/163-replace-reward-account-with-stak…

f54e8f7

…e-address # Conflicts: # common/src/address.rs # common/src/stake_addresses.rs # common/src/types.rs # modules/accounts_state/src/state.rs # modules/tx_unpacker/src/tx_unpacker.rs

lowhung force-pushed the lowhung/163-replace-reward-account-with-stake-address branch from 78bb61a to d19a507 Compare October 22, 2025 23:42

Consolidated #[derive(...)] traits for Credential enum

9669139

lowhung force-pushed the lowhung/163-replace-reward-account-with-stake-address branch from d19a507 to 9669139 Compare October 22, 2025 23:43

Refactor reward account handling to use StakeAddress #250

Are you sure you want to change the base?

Refactor reward account handling to use StakeAddress #250

Uh oh!

Conversation

lowhung commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

Uh oh!

sandtreader left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lowhung Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lowhung Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lowhung commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lowhung commented Oct 14, 2025 •

edited

Loading

lowhung Oct 20, 2025 •

edited

Loading

lowhung Oct 20, 2025 •

edited

Loading

lowhung commented Oct 20, 2025 •

edited

Loading